server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java-132-148 (1)

132-148: ⚠️ Potential issue | 🟠 Major

Validate/sanitize extension to prevent path traversal in storeBytes.

extension is concatenated into the filename without any clean-path or .. guard, so a crafted value could escape the upload directory.

🔒 Suggested fix

 	public String storeBytes(byte[] bytes, String extension, int maxSize) {
 		try {
 			if (bytes == null || bytes.length == 0) {
 				throw new UploadException("Failed to store empty file");
 			}
@@
 			MessageDigest digest = MessageDigest.getInstance("SHA-256");
 			byte[] hashBytes = digest.digest(bytes);
 			String hash = HexFormat.of().formatHex(hashBytes);
-			String filename = hash + "." + extension;
+			if (!StringUtils.hasText(extension)
+					|| extension.contains("..")
+					|| extension.contains("/")
+					|| extension.contains("\\")) {
+				throw new UploadException("File extension not allowed");
+			}
+			String filename = StringUtils.cleanPath(hash + "." + extension.toLowerCase());
+			if (filename.contains("..")) {
+				throw new UploadException("Cannot store file with relative path outside current directory");
+			}
 
 			Files.write(rootLocation.resolve(filename), bytes);
 			return filename;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java` around
lines 132 - 148, The storeBytes method currently concatenates the provided
extension into the filename which allows path traversal; sanitize or validate
the extension before building filename in StoreService.storeBytes: reject or
normalize any extension containing path-separators, dots, or unexpected
characters and only allow a safe whitelist (e.g., alphanumeric, max length) or
map to known allowed extensions; then construct the filename using the computed
hash + "." + the validated extension and write to rootLocation.resolve(filename)
so no crafted extension can escape the upload directory.

server/src/main/java/de/tum/cit/aet/thesis/cron/ProfilePictureMigration.java-70-71 (1)

70-71: ⚠️ Potential issue | 🟠 Major

Avoid logging user identifiers (PII) at INFO level.

user.getUniversityId() is a personal identifier (the user's TUM username). Logging it at INFO level means it lands in persistent production logs, creating an unnecessary PII retention risk.

Drop getUniversityId() from the log — the UUID is sufficient for correlation.

🛡️ Proposed fix

-					log.info("Profile picture migration: downloaded avatar for user {} ({})",
-							user.getUniversityId(), user.getId());
+					log.info("Profile picture migration: downloaded avatar for user {}", user.getId());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/cron/ProfilePictureMigration.java`
around lines 70 - 71, The log entry in ProfilePictureMigration currently logs a
personal identifier via user.getUniversityId(); remove that call from the log
message and only log the UUID (user.getId()) to avoid persisting PII at INFO
level — update the log.info invocation in ProfilePictureMigration to exclude
user.getUniversityId() and ensure the message and formatting only reference
user.getId().

server/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionResultDto.java-1-6 (1)

1-6: ⚠️ Potential issue | 🟠 Major

Add @JsonInclude(NON_EMPTY) annotation to this DTO.

All DTOs in this module require the annotation to omit null, empty strings, and empty collections from JSON serialization.

Suggested fix

+import com.fasterxml.jackson.annotation.JsonInclude;
+
+@JsonInclude(JsonInclude.Include.NON_EMPTY)
 public record UserDeletionResultDto(
 		String result,
 		String message
 ) {
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionResultDto.java`
around lines 1 - 6, Add the Jackson annotation to the UserDeletionResultDto
record so empty/null values are omitted in JSON: annotate the record
UserDeletionResultDto with `@JsonInclude`(JsonInclude.Include.NON_EMPTY) and add
the necessary import for com.fasterxml.jackson.annotation.JsonInclude; place the
annotation immediately above the record declaration to match other DTOs in the
module.

server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java-174-174 (1)

174-174: 🛠️ Refactor suggestion | 🟠 Major

Multiple TODO comments left in production code.

Eleven identical TODO comments were added throughout the file. Per coding guidelines, TODO comments should not remain in production code. If there's a genuine concern about @Transactional usage, track it as an issue/ticket rather than scattering TODOs across the codebase.

As per coding guidelines: "Avoid leaving TODO comments in production code."

Also applies to: 208-208, 223-223, 277-277, 322-322, 393-393, 412-412, 429-429, 447-447, 455-455, 489-489

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java`
at line 174, Remove the eleven TODO comments about avoiding `@Transactional` from
the ApplicationService class; instead open a tracked issue/ticket and replace
each TODO with a short, non-actionable comment referencing that ticket ID (or
remove the comment entirely) so production code contains no TODOs. Locate the
comments in ApplicationService around the methods annotated with `@Transactional`
and remove or replace them with a one-line reference to the created issue (e.g.,
"See ISSUE-XXXX for discussion on `@Transactional` usage") to preserve
traceability without leaving TODOs in production.

server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsApplicationEmailDTO.java-5-11 (1)

5-11: 🛠️ Refactor suggestion | 🟠 Major

Missing @JsonInclude(JsonInclude.Include.NON_EMPTY) annotation.

Per coding guidelines and project convention, all DTOs must include this annotation.

Fix

 package de.tum.cit.aet.thesis.dto;
 
+import com.fasterxml.jackson.annotation.JsonInclude;
 import de.tum.cit.aet.thesis.entity.ResearchGroupSettings;
 
+@JsonInclude(JsonInclude.Include.NON_EMPTY)
 public record ResearchGroupSettingsApplicationEmailDTO(
 		boolean includeApplicationDataInEmail
 ) {

As per coding guidelines: "All DTOs must use @JsonInclude(JsonInclude.Include.NON_EMPTY) annotation; null, empty strings, and empty collections are omitted from JSON serialization." Based on learnings from @krusche in PR #820: "Apply JsonInclude(JsonInclude.Include.NON_EMPTY) to all DTOs to minimize payload size."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsApplicationEmailDTO.java`
around lines 5 - 11, Add the missing Jackson annotation to the DTO: annotate the
ResearchGroupSettingsApplicationEmailDTO record with
`@JsonInclude`(JsonInclude.Include.NON_EMPTY) so that JSON serialization omits
nulls, empty strings and empty collections; update imports accordingly and
ensure the annotation is applied on the record declaration
(ResearchGroupSettingsApplicationEmailDTO) to follow project DTO conventions.

server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java-521-532 (1)

521-532: ⚠️ Potential issue | 🟠 Major

Remove the explicit reviewer cleanup and rely on the CASCADE FK from migration 27.

The deleteApplication method explicitly deletes reviewers before deleting the application (lines 529–530), then deletes the application itself (line 531). Since the foreign key constraint added in migration 27 specifies ON DELETE CASCADE, these manual operations are redundant and create an unnecessary data consistency risk: if the application delete fails, the reviewers are already gone.

Simplify to:

Suggested fix

 public void deleteApplication(UUID applicationId) {
     Application application = findById(applicationId);
 
     if (application.getState() == ApplicationState.ACCEPTED) {
         throw new ResourceInvalidParametersException(
                 "Accepted applications cannot be deleted because they are linked to a thesis.");
     }
 
-    applicationReviewerRepository.deleteAll(application.getReviewers());
-    application.getReviewers().clear();
     applicationRepository.delete(application);
 }

The cascade will handle reviewer deletion automatically and atomically at the database level.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java`
around lines 521 - 532, The deleteApplication method in ApplicationService
currently manually deletes reviewers via
applicationReviewerRepository.deleteAll(application.getReviewers()) and
application.getReviewers().clear() before calling
applicationRepository.delete(application); remove those two explicit reviewer
cleanup calls and rely on the DB-level ON DELETE CASCADE from migration 27 so
that deleting the Application (in deleteApplication(UUID applicationId)) will
atomically remove related reviewers; keep the existing accepted-state check and
the final applicationRepository.delete(application) call.

server/src/main/resources/db/changelog/changes/28_data_export.sql-4-12 (1)

4-12: 🛠️ Refactor suggestion | 🟠 Major

Missing index on user_id foreign key.

Queries for a user's export status and nightly cleanup jobs will scan the full table without an index on user_id.

✅ Proposed fix

 CREATE TABLE data_exports (
     data_export_id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
     user_id UUID NOT NULL REFERENCES users(user_id),
     state TEXT NOT NULL DEFAULT 'REQUESTED',
     file_path TEXT,
     created_at TIMESTAMP NOT NULL DEFAULT NOW(),
     creation_finished_at TIMESTAMP,
     downloaded_at TIMESTAMP
 );
+
+CREATE INDEX idx_data_exports_user_id ON data_exports(user_id);

As per coding guidelines: "Add proper indexes for foreign keys and frequently queried columns."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/resources/db/changelog/changes/28_data_export.sql` around
lines 4 - 12, Add an index on the data_exports.user_id foreign key to avoid
full-table scans when querying a user's exports or running cleanup jobs: update
the migration that creates the data_exports table (table name: data_exports,
column: user_id) to include a CREATE INDEX for user_id; optionally also add a
composite index on (user_id, state) if your queries/filtering frequently include
the state column (state) to speed status lookups and cleanup.

server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsWritingGuideDTO.java-1-11 (1)

1-11: 🛠️ Refactor suggestion | 🟠 Major

Missing @JsonInclude(JsonInclude.Include.NON_EMPTY) annotation.

Without it, a null scientificWritingGuideLink will be serialized as "scientificWritingGuideLink": null instead of being omitted from the payload.

✅ Proposed fix

 package de.tum.cit.aet.thesis.dto;

+import com.fasterxml.jackson.annotation.JsonInclude;
 import de.tum.cit.aet.thesis.entity.ResearchGroupSettings;

+@JsonInclude(JsonInclude.Include.NON_EMPTY)
 public record ResearchGroupSettingsWritingGuideDTO(
         String scientificWritingGuideLink
 ) {

As per coding guidelines: "All DTOs must use @JsonInclude(JsonInclude.Include.NON_EMPTY) annotation." Also based on learnings from PR #820: "Apply JsonInclude(JsonInclude.Include.NON_EMPTY) to all DTOs to minimize payload size."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsWritingGuideDTO.java`
around lines 1 - 11, Add the Jackson non-empty inclusion annotation to the DTO
so null/empty scientificWritingGuideLink is omitted: import
com.fasterxml.jackson.annotation.JsonInclude and annotate the record
ResearchGroupSettingsWritingGuideDTO with
`@JsonInclude`(JsonInclude.Include.NON_EMPTY); leave the existing record
components and the fromEntity(ResearchGroupSettings settings) method unchanged
so serialization behavior is adjusted without altering construction logic.

server/src/test/java/de/tum/cit/aet/thesis/service/MailingServiceIntegrationTest.java-140-182 (1)

140-182: ⚠️ Potential issue | 🟠 Major

The test's core assertion is missing — test name is misleading.

createApplication_WithSettingEnabled_ChairEmailHasAttachments never asserts that the chair email actually has attachments. The only assertions are that at least one email was sent and the student received one, which are identical to the baseline test. The countAttachments helper is never called here.

✅ Proposed fix — add the chair attachment assertion

         assertThat(allRecipients).as("Student should receive an email")
                 .anyMatch(addr -> addr.contains(student.universityId()));
+
+        // Chair email should include attachments when setting is enabled
+        boolean chairEmailVerified = false;
+        for (MimeMessage email : emails) {
+            List<String> recipients = Arrays.stream(email.getAllRecipients())
+                    .map(Address::toString)
+                    .toList();
+            if (recipients.stream().anyMatch(addr -> addr.contains(head.universityId()))) {
+                assertThat(countAttachments(email))
+                        .as("Chair email should have attachments when includeApplicationDataInEmail is enabled")
+                        .isGreaterThan(0);
+                chairEmailVerified = true;
+            }
+        }
+        assertThat(chairEmailVerified).as("At least one chair email should have been found and verified").isTrue();
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@server/src/test/java/de/tum/cit/aet/thesis/service/MailingServiceIntegrationTest.java`
around lines 140 - 182, The test
createApplication_WithSettingEnabled_ChairEmailHasAttachments doesn't assert
that the chair's email contains attachments; update the test to locate the
MimeMessage for the chair (use head.universityId() and
getReceivedEmails()/getAllRecipientAddresses() to find the message) and call the
existing countAttachments helper on that MimeMessage, then assert the attachment
count is greater than zero (e.g., assertTrue or
assertThat(countAttachments(chairMessage)).isGreaterThan(0)). Ensure you
reference the createApplication_WithSettingEnabled_ChairEmailHasAttachments
test, the head variable, getReceivedEmails(), getAllRecipientAddresses(), and
countAttachments() when making the change.

server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java-77-98 (1)

77-98: ⚠️ Potential issue | 🟠 Major

The includeData toggle only controls attachment inclusion, not email body content.

Both prepareApplicationCreatedMailBuilder and prepareMinimalApplicationMailBuilder call the identical fillApplicationPlaceholders(application) method, which injects the complete MailApplication record into the template. This exposes sensitive data (motivation, special skills, interests, email, university ID, matriculation number, study program/degree) in the email body regardless of the toggle setting. Only the attachments (CV, examination report, degree report) are conditionally included.

Consider using a dedicated minimal template without application placeholders, or create a separate placeholder-filling method that only injects essential fields when includeData is false.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java`
around lines 77 - 98, The includeData toggle only controls attachments but not
the email body: both prepareApplicationCreatedMailBuilder and
prepareMinimalApplicationMailBuilder call
fillApplicationPlaceholders(application) and therefore leak sensitive fields;
change the logic so the minimal path does not inject full application
data—either load a dedicated minimal EmailTemplate (e.g.,
"APPLICATION_CREATED_CHAIR_MINIMAL") in sendApplicationCreatedEmail or implement
a new filler (e.g., fillMinimalApplicationPlaceholders(Application)) and call it
from prepareMinimalApplicationMailBuilder; ensure sendNotificationCopy also uses
the minimal builder/template when includeData is false so both the main send and
notification copy omit sensitive placeholders.

client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx-32-40 (1)

32-40: ⚠️ Potential issue | 🟠 Major

Guard against missing applicationEmailSettings in the response.

If the server omits applicationEmailSettings (e.g., older records or NON_EMPTY DTOs), this access can throw. Please use optional chaining + a default.

Suggested change

-        if (
-          res.data.applicationEmailSettings.includeApplicationDataInEmail !==
-          includeApplicationDataInEmail
-        ) {
-          setIncludeApplicationDataInEmail(
-            res.data.applicationEmailSettings.includeApplicationDataInEmail,
-          )
-        }
+        const serverValue =
+          res.data?.applicationEmailSettings?.includeApplicationDataInEmail ?? false
+        if (serverValue !== includeApplicationDataInEmail) {
+          setIncludeApplicationDataInEmail(serverValue)
+        }

As per coding guidelines: Client code must handle missing DTO fields with ?? '' for strings, ?? [] for arrays, and ?. for optional chaining.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx`
around lines 32 - 40, Guard against missing applicationEmailSettings in the
response by using optional chaining and a sensible default when reading
res.data.applicationEmailSettings; update the comparison and setter inside
ApplicationEmailContentSettingsCard (the block that references
includeApplicationDataInEmail and calls setIncludeApplicationDataInEmail) to
read res.data.applicationEmailSettings?.includeApplicationDataInEmail with a
fallback (e.g., ?? false) so the code won’t throw when applicationEmailSettings
is absent.

client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx-20-72 (1)

20-72: ⚠️ Potential issue | 🟠 Major

Avoid leaving the toggle in a wrong state when the update fails (and guard missing route param).

Right now the UI stays toggled even if the request fails, which can mislead users about the actual server setting. Also guard against an undefined researchGroupId to avoid /undefined requests.

Suggested change

-  const handleChange = (value: boolean) => {
-    doRequest<IResearchGroupSettings>(
+  const handleChange = (value: boolean) => {
+    if (!researchGroupId) {
+      showSimpleError('Research group not found.')
+      return
+    }
+    const previousValue = includeApplicationDataInEmail
+    setIncludeApplicationDataInEmail(value)
+    doRequest<IResearchGroupSettings>(
       `/v2/research-group-settings/${researchGroupId}`,
       {
         method: 'POST',
         requiresAuth: true,
         data: {
           applicationEmailSettings: {
             includeApplicationDataInEmail: value,
           },
         },
       },
       (res) => {
         if (res.ok) {
-          if (
-            res.data.applicationEmailSettings.includeApplicationDataInEmail !==
-            includeApplicationDataInEmail
-          ) {
-            setIncludeApplicationDataInEmail(
-              res.data.applicationEmailSettings.includeApplicationDataInEmail,
-            )
-          }
+          const serverValue =
+            res.data?.applicationEmailSettings?.includeApplicationDataInEmail ?? false
+          setIncludeApplicationDataInEmail(serverValue)
         } else {
+          setIncludeApplicationDataInEmail(previousValue)
           showSimpleError(getApiResponseErrorMessage(res))
         }
       },
     )
   }
@@
-          <Switch
-            checked={includeApplicationDataInEmail}
-            onChange={(event) => {
-              setIncludeApplicationDataInEmail(event.currentTarget.checked)
-              handleChange(event.currentTarget.checked)
-            }}
-          />
+          <Switch
+            checked={includeApplicationDataInEmail}
+            onChange={(event) => handleChange(event.currentTarget.checked)}
+          />

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx`
around lines 20 - 72, The toggle is left in the wrong state when the API update
fails and we also send requests to /undefined if researchGroupId is missing; fix
by guarding researchGroupId and reverting the optimistic UI on failures: in the
Switch onChange handler capture the previous value (const prev =
includeApplicationDataInEmail), check if researchGroupId is defined and if not
call showSimpleError/getApiResponseErrorMessage and reset state to prev, then
call handleChange; inside handleChange (or the doRequest callback) handle non-ok
responses and network errors by calling setIncludeApplicationDataInEmail(prev)
to revert the toggle and showSimpleError(getApiResponseErrorMessage(res)) (and
add a catch around doRequest to revert on exceptions). Reference symbols: Switch
onChange handler, includeApplicationDataInEmail,
setIncludeApplicationDataInEmail, handleChange, doRequest, researchGroupId,
showSimpleError, getApiResponseErrorMessage.

client/src/pages/DataExportPage/DataExportPage.tsx-1-8 (1)

1-8: ⚠️ Potential issue | 🟠 Major

Use named export and replace raw HTML div with Mantine component.

Line 194: Change export default DataExportPage to export { DataExportPage } and update imports accordingly.

Line 120: Replace the outer <div> wrapper with a Mantine component like <Stack> or remove it if the inner <Stack> (line 123) suffices.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/src/pages/DataExportPage/DataExportPage.tsx` around lines 1 - 8,
Change the default export to a named export by replacing "export default
DataExportPage" with "export { DataExportPage }" and update any imports
elsewhere to use the named import; also remove or replace the outer <div>
wrapper around the component's JSX (the wrapper around the inner Stack at the
top of the DataExportPage render) with a Mantine layout component such as
<Stack> (or delete the outer div if the inner <Stack> suffices) so the component
uses Mantine primitives consistently and avoids raw HTML containers.

client/src/pages/SettingsPage/components/AccountDeletion/AccountDeletion.tsx-80-81 (1)

80-81: ⚠️ Potential issue | 🟠 Major

Confirmation bypass when user name is empty.

If auth.user?.firstName and auth.user?.lastName are both undefined/null, fullName resolves to "" and confirmName starts as "", so confirmName !== fullName is false — the delete button is enabled without typing anything.

🐛 Proposed guard

   const fullName = `${auth.user?.firstName ?? ''} ${auth.user?.lastName ?? ''}`.trim()
   const canDelete = !preview.hasActiveTheses && !preview.isResearchGroupHead
+  const hasValidName = fullName.length > 0

   ...

-            <Button color='red' disabled={confirmName !== fullName} onClick={onDelete}>
+            <Button color='red' disabled={!hasValidName || confirmName !== fullName} onClick={onDelete}>

Also applies to: 161-161

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/src/pages/SettingsPage/components/AccountDeletion/AccountDeletion.tsx`
around lines 80 - 81, The delete confirmation can be bypassed when fullName is
empty; update the logic around fullName, confirmName and canDelete so deletion
requires an explicit non-empty match: ensure fullName (computed in
AccountDeletion as fullName) is treated as empty when both names missing and
include a guard like fullName.length > 0 in the canDelete condition (i.e., only
allow deletion if !preview.hasActiveTheses && !preview.isResearchGroupHead &&
fullName !== '' && confirmName === fullName), or alternatively require a fixed
token (e.g., typing "DELETE") when fullName is empty; adjust any UI/initial
state for confirmName accordingly so an empty string does not satisfy the
confirmation.

server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java-157-170 (1)

157-170: ⚠️ Potential issue | 🟠 Major

Entire deferred-deletion batch runs in a single @Transactional — one failure rolls back all.

If performFullDeletion throws for one user, the whole batch is rolled back. Consider per-user transaction boundaries (e.g., inject self via ApplicationContext and call a @Transactional(propagation = REQUIRES_NEW) method, or use TransactionTemplate).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java`
around lines 157 - 170, processDeferredDeletions currently runs the entire batch
in one `@Transactional` so a failure in performFullDeletion for one user rolls
back all; change to per-user transaction boundaries by moving the deletion work
into a separate method annotated with `@Transactional`(propagation = REQUIRES_NEW)
(e.g., create performFullDeletionTransactional or similar) and invoke it via the
Spring proxy (inject self via ApplicationContext or a self bean) or use
TransactionTemplate to execute each user's deleteAllUserFiles, deleteDataExports
and performFullDeletion inside its own new transaction after confirming
getRetentionBlockedThesisRoles is empty; ensure processDeferredDeletions loops
and calls the proxied/new-transaction method so failures for one user do not
rollback others.

server/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.java-382-401 (1)

382-401: ⚠️ Potential issue | 🟠 Major

InputStream leak in addUserFile — stream from resource.getInputStream() is never closed.

FileSystemResource.getInputStream() opens a new FileInputStream each time. transferTo does not close the source stream. This leaks file handles, which can exhaust the OS limit during large exports.

Proposed fix

 	private void addUserFile(ZipOutputStream zos, String filename, String entryPrefix) {
 		if (filename == null || filename.isBlank()) {
 			return;
 		}
 		try {
 			FileSystemResource resource = uploadService.load(filename);
 			if (resource.exists()) {
 				String extension = "";
 				int dotIndex = filename.lastIndexOf('.');
 				if (dotIndex >= 0) {
 					extension = filename.substring(dotIndex);
 				}
 				zos.putNextEntry(new ZipEntry(entryPrefix + extension));
-				resource.getInputStream().transferTo(zos);
+				try (var is = resource.getInputStream()) {
+					is.transferTo(zos);
+				}
 				zos.closeEntry();
 			}
 		} catch (Exception e) {
 			log.warn("Failed to include file {} in export: {}", filename, e.getMessage());
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.java`
around lines 382 - 401, The addUserFile method leaks the InputStream from
resource.getInputStream() because transferTo doesn't close the source; wrap the
InputStream in a try-with-resources (e.g., try (InputStream in =
resource.getInputStream()) { ... }) around the transferTo call so the stream is
always closed, ensure you still call zos.putNextEntry(entryPrefix + extension)
before transfer and zos.closeEntry() after transfer (in a finally or after the
try-with-resources) and avoid closing the ZipOutputStream itself; update
addUserFile to obtain the InputStream via try-with-resources and perform
transferTo(zos) inside that block so file handles are released.

server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java-41-41 (1)

41-41: ⚠️ Potential issue | 🟠 Major

DateTimeFormatter without explicit Locale — month names will vary by JVM locale.

"MMMM d, yyyy" produces locale-dependent month names (e.g., "Januar" on a German JVM). Since this is for user-facing messages, pin a locale:

-	private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("MMMM d, yyyy");
+	private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("MMMM d, yyyy", Locale.ENGLISH);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java`
at line 41, The DateTimeFormatter DATE_FORMATTER in UserDeletionService is
created without an explicit Locale so month names will vary by JVM locale;
update the DATE_FORMATTER initialization to supply an explicit Locale (e.g.,
Locale.ENGLISH or Locale.US) when calling DateTimeFormatter.ofPattern("MMMM d,
yyyy") so the month names are deterministic for user-facing messages.

server/src/main/java/de/tum/cit/aet/thesis/service/ResearchGroupService.java-298-298 (1)

298-298: ⚠️ Potential issue | 🟠 Major

Reference equality (!=) used to compare JPA entities — use ID-based comparison instead.

oldHead != head compares object identity. If the same user is loaded from different Hibernate sessions or detached/reattached, two objects representing the same row can be different references. Compare by ID:

Proposed fix

-		if (oldHead != head) {
+		if (!oldHead.getId().equals(head.getId())) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/ResearchGroupService.java`
at line 298, The check in ResearchGroupService that uses reference comparison
(oldHead != head) must be replaced with an ID-based equality check to handle
detached or reloaded JPA entities: compare the primary key values (e.g.,
Objects.equals(oldHead.getId(), head.getId())) and handle nulls (oldHead or head
may be null) so the logic triggers only when the persisted identity truly
changed; update the condition in the method containing oldHead/head to use
Objects.equals on IDs (and add an import for java.util.Objects if needed).

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

client/playwright.config.ts (1)

8-8: Simplify the redundant ternary — both branches are identical.

process.env.CI ? 2 : 2 always evaluates to 2. The conditional adds noise without any behavioral difference.

♻️ Proposed simplification

-  workers: process.env.CI ? 2 : 2,
+  workers: 2,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/playwright.config.ts` at line 8, The workers setting uses a redundant
ternary expression (process.env.CI ? 2 : 2) which always evaluates to 2;
simplify the Playwright config by replacing that expression with a plain numeric
assignment (workers: 2) and remove the unnecessary conditional to reduce noise
while keeping behavior identical, locating the change at the workers property in
client/playwright.config.ts.

client/e2e/application-review-workflow.spec.ts (1)

11-14: Silent pass on load failure in a serial suite could mask cascading issues.

In a serial suite, if the first test silently returns because navigateToDetail fails, the second test likely also skips silently. Both tests "pass" but nothing was actually verified. Consider logging a warning or using test.skip() instead of return so the test report surfaces that these were not actually exercised.

Suggested approach

     const loaded = await navigateToDetail(page, `/applications/${APPLICATION_REJECT_ID}`, heading)
-    if (!loaded) return // Application not accessible (may have been modified by a parallel test)
+    if (!loaded) {
+      test.skip(true, 'Application detail page did not load — skipping')
+      return
+    }

Also applies to: 51-54

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/e2e/application-review-workflow.spec.ts` around lines 11 - 14, The
test silently returning when navigateToDetail(...) fails hides skipped work;
replace the early "if (!loaded) return" with a visible skip so the test runner
reports it—call test.skip(true, `navigateToDetail failed for
${APPLICATION_REJECT_ID}`) (or test.skip(true, 'navigateToDetail failed') if
string interpolation is inconvenient) when loaded is false, then return; update
the same pattern around the other occurrence (the block using navigateToDetail,
heading, and APPLICATION_REJECT_ID at lines ~51-54) so failures are surfaced
instead of silently passing.

client/src/components/UserMultiSelect/UserMultiSelect.tsx (1)

4-5: Use type-only imports for DTO types.

These imports are only used in type positions; switch to import type to match TS best practices and repo rules.

♻️ Suggested change

-import { PaginationResponse } from '../../requests/responses/pagination'
-import { ILightUser } from '../../requests/responses/user'
+import type { PaginationResponse } from '../../requests/responses/pagination'
+import type { ILightUser } from '../../requests/responses/user'

As per coding guidelines, Use 'import type' for type-only imports.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx` around lines 4 -
5, The imports PaginationResponse and ILightUser in UserMultiSelect.tsx are used
only as types; change their declarations to use TypeScript's type-only import
form (e.g., "import type { PaginationResponse } ..." and "import type {
ILightUser } ...") so the compiler and bundler know they're erased at runtime;
update the import line importing PaginationResponse and ILightUser accordingly.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

client/e2e/navigation.spec.ts (1)

72-72: Timeout inconsistency: 30 s vs 15 s used everywhere else in this file.

Every other page-level assertion here uses { timeout: 15_000 }. Bumping only this one to 30_000 silently accepts that the back-navigation to /dashboard is twice as slow as any other transition, which could mask a real performance regression introduced by this PR's new features (avatar migration, retention settings, etc.). If the higher value is genuinely needed, a brief inline comment explaining why would prevent it from being cargo-culted further.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/e2e/navigation.spec.ts` at line 72, The test uses an inconsistent
timeout for the page URL assertion (await expect(page).toHaveURL(/\/dashboard/,
{ timeout: 30_000 })); change it to match the rest of the file by using {
timeout: 15_000 } or, if 30_000 is actually required, add a short inline comment
explaining why this navigation needs the longer timeout (mentioning page and
toHaveURL to locate the assertion).

server/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java (1)

15-18: Consider adding @Param for consistency with the rest of the codebase.

The -parameters compiler flag is enabled in the build configuration, so Spring Data JPA can bind :userId without explicit @Param. However, most other @Query methods in the repository layer use @Param annotations. Adding it here improves consistency and clarity.

Optional alignment

 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.Modifying;
 import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;
 import org.springframework.transaction.annotation.Transactional;
@@
-	void deleteByUserId(UUID userId);
+	void deleteByUserId(`@Param`("userId") UUID userId);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@server/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java`
around lines 15 - 18, The deleteByUserId method declaration lacks an explicit
`@Param` on its UUID parameter, which is inconsistent with other repository
queries; update the method signature for deleteByUserId(UUID userId) to annotate
the parameter with `@Param`("userId") (and add the import if missing) so the
`@Query`(":userId") binding is explicit and consistent with the rest of the
repository.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

client/e2e/data-retention.spec.ts (1)

18-24: Prefer explicit test skip over silent return.

A bare return makes the test pass, which can mask missing fixtures. Use test.skip so the report shows a skip with reason.

🔧 Suggested change

-    if (!hasApplication) {
-      // Application was already deleted in a prior run — skip gracefully
-      return
-    }
+    if (!hasApplication) {
+      // Application was already deleted in a prior run — skip gracefully
+      test.skip(true, 'Application already deleted in a prior run')
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/e2e/data-retention.spec.ts` around lines 18 - 24, The code silently
returns when the application heading isn't found (variables: heading,
hasApplication, page.getByRole), which masks missing fixtures; change the
behavior to explicitly skip the test by calling test.skip when hasApplication is
false (e.g., await the waitFor as now to set hasApplication, then replace the
bare return with test.skip(!hasApplication, 'Application was already deleted in
a prior run') so the test runner reports a skipped test rather than a false
pass).

server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java (1)

81-103: Consider indexing for nightly cleanup performance.
This query is likely run in batch cleanup; ensure indexes exist on UserGroup.userId, ThesisRole.user.id, and Application.user.id to keep it fast at scale.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java`
around lines 81 - 103, The query in findInactiveStudentCandidates may be slow
during nightly cleanup because it scans joins and NOT EXISTS on related tables;
add database indexes on the referenced foreign/key columns to speed those checks
— specifically create/index UserGroup.id.userId (or UserGroup.user_id),
ThesisRole.user.id (or thesis_role.user_id), and Application.user.id (or
application.user_id); also ensure indexes on User.lastLoginAt, User.updatedAt
(or their persisted column names) if filtering by cutoff is frequent; update the
migration or schema changes accordingly so the DB has these indexes before
running the batch job.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro